Will an end user/student see this error message? If so, I'd change it to indicate what "save your state" means - for example, "We can't currently save your note. Make sure that you are connected to the Internet, and then try refreshing the page."

Also: I've added this to the Google doc so all the UI text recommendations would be in one place.

Will an end user/student see this error message? If so, I'd change it to indicate what "save your state" means - for example, "We can't currently save your note. Make sure that you are connected to the Internet, and then try refreshing the page."

Yes, it'll. This functionality is responsible for toggling visibility of the notes (https://openedx.atlassian.net/browse/TNL-661). It is not about note saving.

OK, I'd go with this (assuming we don't want to just use the generic "An error has occurred," which I think would be fine here):

We couldn't <show/hide> your notes. Make sure that you are connected to the Internet, and then try refreshing the page.

@srpearce

Changed the error message to:
We couldn't <show/hide> your notes. Make sure that you are connected to the Internet, and then try refreshing the page. Just to be sure, the < > around show/hide are intentional?

Also, did the same change to the error message in toggle_notes_factory_spec.js

I don't understand why there is a second method (which looks very much like _config_course_advanced_components). Please explain why there are 2 methods, and make sure the comments in this method are all correct (there are references to advanced settings, but I don't think this method is supposed to be about advanced settings).

I don't understand why there is a second method (which looks very much like _config_course_advanced_components). Please explain why there are 2 methods, and make sure the comments in this method are all correct (there are references to advanced settings, but I don't think this method is supposed to be about advanced settings).

_config_course_advanced_components is more specific and works only with advanced_modules field. It adds/removes appropriate tab when specific module is added.
_config_course_settings has the similar logic, but works with course fields.

We'll update names and comments.

this has been reworked in edx@c384ee5

afaict, this is simpler and the same

while xblock:
   parent = xblock.get_parent()
   if parent is None:
       return None
   if parent.category == 'sequential':
       return xblock
   xblock = parent

indeed.

Unfortunately you have to update all your icons to use the new font-awesome scheme (fa-check). If you rebase you'll find that all these icons are broken, as the new version of font-awesome was merged last week.

Unfortunately you have to update all your icons to use the new font-awesome scheme (fa-check). If you rebase you'll find that all these icons are broken, as the new version of font-awesome was merged last week.

Ah, we forgot to update the icons in our test fixtures. Let me fix that.

The code looks right to me, but I don't know the pitfalls. I don't think you can merge with this, though, without having unit tests that verify that it does work as expected. Can you reuse the tests from Jim's branch?

For example, does the cache get cleared appropriately when an xblock is reparented?

as the cache is stored in request_cache, it is cleared after each request completes.

I know, but what happens if code accesses the parent after a reparent within the same request. I guess that's more of a problem for @jimabramson's generic solution than for yours, since you know where you are using this method.

Anyway, there need to be tests of the basic cases. What happens when called on a block which hasn't been parented yet? What happens with the boundary between a draft child and a non-draft parent? Are there gotchas in split vs old mongo?

Thinking some more, I suppose you don't have to worry about drafts as your method is only in LMS...

a test was added edx@19dc8ec